feat: proxy support for the new downloader #5795
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 4eaeb32 in 2 minutes and 30 seconds. Click for details.
- Reviewed
1531lines of code in9files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/download-extension/src/index.ts:31
- Draft comment:
Good integration: proxy config is now passed to downloadFiles in downloadFile. Consider documenting the expected proxy config shape in a code comment for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. extensions/llamacpp-extension/src/backend.ts:129
- Draft comment:
Proxy config is integrated into backend download items. Ensure that getProxyConfig behavior is consistent across extensions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. extensions/llamacpp-extension/src/index.ts:611
- Draft comment:
Proxy config is now passed in model downloads. This is consistent; ensure any proxy setting changes update the UI accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. extensions/llamacpp-extension/src/util.test.ts:40
- Draft comment:
Comprehensive tests for getProxyConfig. Consider extracting common mock setup into a shared test helper to reduce repetition. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. extensions/llamacpp-extension/src/util.ts:64
- Draft comment:
Test expecting console.log output: getProxyConfig does not log any details. Either add logging or update the test accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment seems to misunderstand the code - there is logging where it matters (error cases). The function is a utility that returns a config object, it doesn't need to log successful operations. The comment appears to be making assumptions about test requirements without strong evidence. We don't have context about what test they're referring to. Maybe there is a test somewhere that expects console.log output that I can't see? Maybe there's a logging standard in the codebase that requires logging successful operations? Without seeing the test in question or documentation about logging standards, we can't verify if this comment is valid. The function already logs errors appropriately. The comment should be deleted as it makes assumptions about testing/logging requirements without clear evidence, and the function already has appropriate error logging.
6. src-tauri/src/core/utils/download.rs:49
- Draft comment:
Proxy validation functions are well implemented. Consider caching the parsed URL to avoid double parsing in validate_proxy_config. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. src-tauri/src/core/utils/download.rs:160
- Draft comment:
Using proxy_config.no_proxy.as_deref().unwrap_or(&[]) is clear. Ensure that the wildcard matching in should_bypass_proxy meets enterprise requirements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. web-app/src/routes/settings/https-proxy.tsx:44
- Draft comment:
Simplified proxy settings UI by removing calls to configurePullOptions; verify that downstream consumers pick up proxy settings from localStorage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
9. web-app/src/services/models.ts:28
- Draft comment:
No specific changes regarding proxy support here; just ensure model operations are unaffected by proxy configuration changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_Yf2n091BKdZpThSO
You can customize 
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b7be879 in 1 minute and 1 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/services/__tests__/models.test.ts:247
- Draft comment:
Removed tests for 'configurePullOptions'. Ensure that proxy configuration functionality is covered elsewhere to maintain test coverage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests ensuring that the proxy configuration functionality is covered elsewhere, which is a form of asking the author to ensure something is tested. This violates the rule against asking the author to ensure the change is tested.
Workflow ID: wflow_8I9rnxIguoi2GGb5
You can customize
Barecheck - Code coverage reportTotal: 35.35%Your code coverage diff: 0.03% ▴ |
louis-jan
force-pushed
the
feat/proxy-support-for-the-new-downloader
branch
from
b7be879 to
b76b1d8
Compare
louis-jan
force-pushed
the
feat/proxy-support-for-the-new-downloader
branch
2 times, most recently
from
85495ca to
b7be879
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b5ab1ae in 1 minute and 35 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/jan-linter-and-test.yml:114
- Draft comment:
Removed redundant 'cargo install tauri-driver --locked' steps. Please confirm that tauri-driver is no longer needed in these jobs or is installed elsewhere to avoid breaking any tests. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_fD0RI9wdrTOkwoAN
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add proxy support for the new downloader
Summary
Changes
ProxyConfigstruct with SSL verification and no-proxy list supportCleanShot.2025-07-17.at.00.21.39.mp4
Test plan
Important
Added proxy support for the Rust-based downloader, integrated proxy settings into llamacpp extension, and updated web app with comprehensive tests.
ProxyConfigstruct indownload.rswith SSL verification and no-proxy list support.llamacpp-extension.util.test.tsfor proxy configuration handling.download.rsfor proxy validation and client creation.tauri-driverinstallation fromjan-linter-and-test.yml.urldependency inCargo.tomlfor URL parsing.This description was created by
for b5ab1ae. You can customize this summary. It will automatically update as commits are pushed.